Fix missing release notes#2413
Conversation
The linuxdoc tool used to generate release notes is not optional.
yadij
added
S-could-use-an-approval
|
This problem has been around for a while, preventing certain types of patching by downstream vendors. |
|
The description doesn't fit the actions - on top of what's in it, it also uses variables for tools. |
|
apart from this, LGTM |
kinkie
added
the
S-waiting-for-author
That is to ensure that tools identified by ./configure (eg when cross-compiling) are used, not anything located in the shell PATH. Updated the description to say that. FWIW, I hit the above problem when testing the change to |
yadij
added
S-waiting-for-reviewer
I agree it's valuable, no question.
I'm not going to ask you to put in the work to separate them, but I do ask you to highlight there's two changes |
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
PR description: Vendors typically use autoreconf to rebuilds Squid ... That tool performs an equivalent to 'make distclean' which erases RELEASENOTES.html.
That (true) statement feels misleading in this PR context because, AFAICT, even make clean erases RELEASENOTES.html (in both official and PR sources): "Vendors" and "autoreconf" could be the motivation for this PR, of course, but the way they are currently mentioned is a red herring -- regular users rebuilding Squid under regular conditions erase RELEASENOTES.html as well. Please rephrase PR description to address this concern.
PR description: Ensuring that file is always able to be re-created if missing.
It is difficult for me to understand what this PR has changed at the top level (other than introducing a linuxdoc availability as make dist success precondition, and even that conclusion requires some guessing!).
Please rephrase PR title and description to be more specific, highlighting the top-level/primary change to RELEASENOTES.html creation targets. For example, "Prior to this changes, make ... created RELEASENOTES.html. Now, make dist creates it." Same for the primary target that erases that generated file (if that target has changed).
| < $< >$@ | ||
| $(SED) \ | ||
| -e "s%[@]SQUID_VERSION[@]%$(VERSION)%g" \ | ||
| -e "s%[@]SQUID_RELEASE[@]%$(SQUID_RELEASE)%g" \ |
There was a problem hiding this comment.
BTW, RELEASENOTES.html still contains an unresolved @PACKAGE_VERSION macro AFAICT. Since this PR removes ENABLE_RELEASE_DOCS protections -- exposing this code to more build cases/environments, it may be a good idea to fix this in this PR, but I do not insist on that.
|
|
||
| AC_PATH_PROG(LINUXDOC, linuxdoc, $FALSE) | ||
| AM_CONDITIONAL(ENABLE_RELEASE_DOCS, test "x${ac_cv_path_LINUXDOC}" != "x$FALSE") | ||
| AC_PATH_PROG(LINUXDOC, linuxdoc, [:]) |
There was a problem hiding this comment.
Please do not set $LINUXDOC binary name to : (when linuxdoc is not found). Doing so is conceptually wrong and creates confusing make dist results.
test `grep -c "@SQUID" release-8.sgml` -eq 0
: -B html -T 2 --split=0 release-8.sgml && \
/usr/bin/perl -i -p -e "s%release-8.html%%" release-8.html
Can't open release-8.html: No such file or directory.
| -e "s%[@]SQUID_RELEASE[@]%$(SQUID_RELEASE)%g" \ | ||
| -e "s%[@]SQUID_RELEASE_OLD[@]%$$(( $(SQUID_RELEASE) - 1 ))%g" \ | ||
| < $< >$@ | ||
| test `grep -c "@SQUID" $@` -eq 0 |
There was a problem hiding this comment.
BTW, this target and/or this particular test is broken: This target should fail when $@ does not exist but it actually succeeds. Since this PR removes ENABLE_RELEASE_DOCS protections -- exposing this code to more build cases/environments, it may be a good idea to fix this in this PR, but I do not insist on that.
| RELEASENOTES.html: html | ||
| cp -p $(builddir)/doc/release-notes/release-$(SQUID_RELEASE).html $@ | ||
|
|
||
| CLEANFILES = RELEASENOTES.html |
There was a problem hiding this comment.
If make dist generates RELEASENOTES.html and make does not, should not that file be cleaned by make distclean instead of make clean?
3 participants
Vendors typically use autoreconf to rebuild Squid after
patching the Makefile.am or configure.ac sources. That tool
performs an equivalent to 'make distclean' which erases
RELEASENOTES.html.
Refactor the RELEASENOTES.html creation using automake rules
with transitive dependency and clean support, and automake
variables for tools detected by ./configure are used for the
build. Ensuring that file is always able to be re-created if
missing.
The need to re-generate RELEASENOTES.html adds
the linuxdoc tool to build dependencies where it was
previously only mandatory for release maintainer
to generate the "Bootstrapped sources" tarball.